Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cheapo WMS support #2409

Closed
wants to merge 1 commit into from
Closed

Cheapo WMS support #2409

wants to merge 1 commit into from

Conversation

tmcw
Copy link
Contributor

@tmcw tmcw commented Apr 8, 2016

2016-04-07 at 10 59 pm

(these changes would be resubmitted against master if people think this is a decent idea, this is more of a proposal than a nearly-finished PR)

This adds a {bbox-epsg-3857} token to URL requests. This would let
us support 3857-supporting CORS-supporting WMS servers in GL JS
with very little change - it would be a minor addition to the style
spec and easy to port to Native.

It's broken in a minor way right now: images aren't correctly
y-positioned through each zoom level. I know this is fixable,
someone with more sleep or fewer cold symptoms could probably get it
working quickly: the conversion from X/Y coords to "pseudo-meters"
or whatever you call 3857 units is wrong in a way that's probably
an off-by-one or something.

Example usage:

    map.addSource('wms-test', {
        "type": "raster",
        "tiles": [
            "http://nowcoast.noaa.gov/arcgis/services/nowcoast/analysis_meteohydro_sfc_qpe_time/MapServer/WmsServer?SERVICE=WMS&REQUEST=GetMap&VERSION=1.1.1&LAYERS=5&STYLES=&FORMAT=image%2Fpng&TRANSPARENT=true&HEIGHT=256&WIDTH=256&SRS=EPSG%3A3857&BBOX={bbox-epsg-3857}",
        ],
        "tileSize": 256
    });

    map.addLayer({
        "id": "wms-test-layer",
        "type": "raster",
        "source": "wms-test",
        "paint": {
        }
    });

Refs #965

@tmcw
Cheapo WMS support
cf90c8c 
This adds a `{bbox-epsg-3857}` token to URL requests. This would let
us support 3857-supporting CORS-supporting WMS servers in GL JS
with very little change - it would be a minor addition to the style
spec and easy to port to Native.

It's broken in a minor way right now: images aren't correctly
y-positioned through each zoom level. I know this is fixable,
someone with more sleep or fewer cold symptoms could probably get it
working quickly: the conversion from X/Y coords to "pseudo-meters"
or whatever you call 3857 units is wrong in a way that's probably
an off-by-one or something.

Refs #965
@bhousel
Copy link
Contributor

bhousel commented Apr 8, 2016

Does it matter that the bbox parameter is constructed differently depending on which version of WMS we're talking to?

@mourner
Copy link
Member

mourner commented Apr 8, 2016

I like the approach. We could drop support for older WMS versions since we're already making assumptions like CORS and EPSG3857. Let's fix the discrepancy in the calculations, and then maybe refactor so that the bbox code only executes if there's a bbox token present in the url. Also, I think we could do away with just bbox, and describe all specifics in the docs. Otherwise we might want to name it bbox-epsg-3857-wms-1-3-or-greater.

@bhousel
Copy link
Contributor

bhousel commented Apr 8, 2016

We can avoid worrying about how to construct the bbox by having different tokens for the bounding edges, like BBOX={lon_min},{lat_min},{lon_max},{lat_max}

@halset
Copy link
Contributor

halset commented Apr 20, 2016

@bhousel This is EPSG:3857 with units in (quasi)meters, not lat/lon degrees. And the (irritating) issue of BBOX parameter ordering between WMS version is for EPSG:4326, not EPSG:3857. So having a single {bbox-epsg-3857} works fine for me.

@indus
Copy link
Contributor

indus commented Apr 22, 2016 •
edited
Loading

If you are willing to add more replacement tokens, it would be nice to also add {-y} for TMS tile scheme support:

.replace('{-y}', Math.pow(2, this.z) - this.y - 1)

(would close #2480 #2565 with smaller impact)

@indus indus mentioned this pull request May 18, 2016
Merged
@bhousel bhousel mentioned this pull request May 23, 2016
Closed
@tmcw
Copy link
Contributor Author

tmcw commented May 26, 2016

Closing in favor of #2612

@tmcw tmcw closed this May 26, 2016
@tmcw tmcw deleted the cheapo-wms branch May 26, 2016 18:45
@Sumbera
Copy link

Sumbera commented Jun 15, 2016

@bhousel, @indus I like your proposal as it is more in-line with the original proposal here #965. I assume If coordinates are swapped, it is easier (and more flexible) to swap tokens than lookup in the documentation for how BBOX parameter have been defined:
{one-with-min-max}
{another-with-max-min}

not to speak about more general getUrl-like API for raster tile that would enable to call any URL with whatever parameter sequence.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@tmcw @bhousel @mourner @halset @indus @Sumbera